Fix crash with nil XPath variables #13

alyssais · 2019-01-29T20:04:44Z

Prior to d91e124, an XPath variable could be set to nil, in which case it would be silently converted to an empty string. This is clearly still the intended behaviour of this method, as can be seen from the (redundant) when nil further down. But, with the introduction of context support, this case was broken, because trying to do @@context[:node] would raise a NoMethodError if there was no context.

I also noticed that then when nil case was redundant, since without it it'll use nil.to_s, which is already the empty string, so I removed that in a seperate commit.

@context

Prior to d91e124, an XPath variable could be set to nil, in which case it would be silently converted to an empty string. This is clearly still the intended behaviour of this method, as can be seen from the `when nil` further down. But, with the introduction of context support, this case was broken, because trying to do @@context[:node] would raise a NoMethodError if there was no context.

This is already covered by #to_s, for which nil returns "".

kou · 2019-01-30T00:55:16Z

Thanks.
Could you add a test for this case?

alyssais · 2019-01-30T17:19:35Z

Actually, is there any reason @@context shouldn't be initialized to {}? That would probably be a more robust fix for this issue.

That wouldn't really be able to be tested, though, because there's no behaviour. Just a change in default value, which a single unit test in the middle of a run couldn't check. What do you think?

If not, I've added a test for the current fix.

kou

Thanks for adding a test.
I could understand the cause of this problem.

We need to distinguish "no argument" case and "passed nil as the argument" case. In this case, we should process for "passed nil as the argument" case. It means that we never use context. We need to use the argument as is.

Here is a right fix:

diff --git a/lib/rexml/functions.rb b/lib/rexml/functions.rb
index 219f9c8..40a3449 100644
--- a/lib/rexml/functions.rb
+++ b/lib/rexml/functions.rb
@@ -13,6 +13,8 @@ module REXML
     @@namespace_context = {}
     @@variables = {}
 
+    NOT_SPECIFIED = Object.new
+
     INTERNAL_METHODS = [
       :namespace_context,
       :namespace_context=,
@@ -135,8 +137,8 @@ module REXML
     #
     # An object of a type other than the four basic types is converted to a
     # string in a way that is dependent on that type.
-    def Functions::string( object=nil )
-      object = @@context[:node] if object.nil?
+    def Functions::string( object=NOT_SPECIFIED )
+      object = @@context[:node] if NOT_SPECIFIED == object
       if object.respond_to?(:node_type)
         case object.node_type
         when :attribute

Actually, is there any reason @@context shouldn't be initialized to {}? That would probably be a more robust fix for this issue.

I think that it's not a robust. It just hides the real problem.

kou · 2019-01-31T21:43:08Z

test/rexml/test_functions.rb

@@ -222,6 +222,22 @@ def test_normalize_space
      assert_equal( [REXML::Comment.new("COMMENT A")], m )
    end

+    def test_string_nil_without_context
+      # Reset to default value
+      REXML::Functions.class_variable_set(:@@context, nil)


We can use REXML::Functions.context = nil.

Could you create a setup method and call this? Because this is needed for all tests.

All tests in this file? Or the entire test suite?

It's enough only for this file.

alyssais · 2019-02-01T10:38:41Z

- def Functions::string( object=nil ) - object = @@context[:node] if object.nil? + def Functions::string( object=NOT_SPECIFIED ) + object = @@context[:node] if NOT_SPECIFIED == object

Could this just be the following?: def Functions::string( object=@@context[:node] )

kou · 2019-02-01T21:06:43Z

It will work too.

kou

Thanks.
I'll merge this.

gsar · 2019-05-04T00:02:02Z

@kou any plans to merge this fix into the next ruby release? thanks!

kou · 2019-05-04T05:02:59Z

This will be included in Ruby 2.7.0.
If you can't wait Ruby 2.7.0, you can use rexml 3.2.1 gem.

## Why? :characters is not normalized in sax2. ## Change - text_unnormalized.rb ``` require 'rexml/document' require 'rexml/parsers/sax2parser' require 'rexml/parsers/pullparser' require 'rexml/parsers/streamparser' xml = <<EOS <root> <A>&ruby#13; Text </A> </root> EOS class Listener def method_missing(name, *args) p [name, *args] end end puts "REXML(DOM)" REXML::Document.new(xml).elements.each("/root/A") {|element| puts element.text} puts "" puts "REXML(Pull)" parser = REXML::Parsers::PullParser.new(xml) while parser.has_next? res = parser.pull p res end puts "" puts "REXML(Stream)" parser = REXML::Parsers::StreamParser.new(xml, Listener.new).parse puts "" puts "REXML(SAX)" parser = REXML::Parsers::SAX2Parser.new(xml) parser.listen(Listener.new) parser.parse ``` ## Before (master) ``` $ ruby text_unnormalized.rb REXML(DOM) Text REXML(Pull) start_element: ["root", {}] text: ["\n ", "\n "] start_element: ["A", {}] text: ["&ruby#13; Text ", "\r Text "] end_element: ["A"] text: ["\n", "\n"] end_element: ["root"] end_document: [] REXML(Stream) [:tag_start, "root", {}] [:text, "\n "] [:tag_start, "A", {}] [:text, "\r Text "] [:tag_end, "A"] [:text, "\n"] [:tag_end, "root"] REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "\r Text "] #<= This [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ``` ## After(This PR) ``` $ ruby text_unnormalized.rb REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "\r Text "] [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ```

… "characters" (#168) ## Why? SAX2 parser expand user-defined entity references and character references but doesn't expand predefined entity references. ## Change - text_unnormalized.rb ``` require 'rexml/document' require 'rexml/parsers/sax2parser' require 'rexml/parsers/pullparser' require 'rexml/parsers/streamparser' xml = <<EOS <root> <A> Text </A> </root> EOS class Listener def method_missing(name, *args) p [name, *args] end end puts "REXML(DOM)" REXML::Document.new(xml).elements.each("/root/A") {|element| puts element.text} puts "" puts "REXML(Pull)" parser = REXML::Parsers::PullParser.new(xml) while parser.has_next? res = parser.pull p res end puts "" puts "REXML(Stream)" parser = REXML::Parsers::StreamParser.new(xml, Listener.new).parse puts "" puts "REXML(SAX)" parser = REXML::Parsers::SAX2Parser.new(xml) parser.listen(Listener.new) parser.parse ``` ## Before (master) ``` $ ruby text_unnormalized.rb REXML(DOM) Text REXML(Pull) start_element: ["root", {}] text: ["\n ", "\n "] start_element: ["A", {}] text: [" Text ", "\r Text "] end_element: ["A"] text: ["\n", "\n"] end_element: ["root"] end_document: [] REXML(Stream) [:tag_start, "root", {}] [:text, "\n "] [:tag_start, "A", {}] [:text, "\r Text "] [:tag_end, "A"] [:text, "\n"] [:tag_end, "root"] REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "\r Text "] #<= This [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ``` ## After(This PR) ``` $ ruby text_unnormalized.rb REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "\r Text "] [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ```

alyssais added 2 commits January 29, 2019 19:55

Remove redundant string substitution for nil

f3423fc

This is already covered by #to_s, for which nil returns "".

Add test for string function with nil context

97047ac

kou requested changes Jan 31, 2019

View reviewed changes

alyssais added 2 commits February 4, 2019 12:16

Differentiate between nil object and no object

2842ce9

Clear Functions.context before each functions test

84fc709

kou approved these changes Feb 9, 2019

View reviewed changes

kou merged commit 2a53c54 into ruby:master Feb 9, 2019

kou mentioned this pull request Feb 9, 2019

XPath: fix default node detection in functions #15

Open

alyssais deleted the nil_context branch February 11, 2019 11:04

gsar mentioned this pull request Mar 10, 2020

.name_id raises NoMethodError in Ruby 2.6 default REXML version SAML-Toolkits/ruby-saml#516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash with nil XPath variables #13

Fix crash with nil XPath variables #13

alyssais commented Jan 29, 2019

kou commented Jan 30, 2019

alyssais commented Jan 30, 2019

kou left a comment

kou Jan 31, 2019

alyssais Feb 1, 2019

kou Feb 1, 2019

alyssais commented Feb 1, 2019 via email

kou commented Feb 1, 2019

kou left a comment

gsar commented May 4, 2019

kou commented May 4, 2019

Fix crash with nil XPath variables #13

Fix crash with nil XPath variables #13

Conversation

alyssais commented Jan 29, 2019

kou commented Jan 30, 2019

alyssais commented Jan 30, 2019

kou left a comment

Choose a reason for hiding this comment

kou Jan 31, 2019

Choose a reason for hiding this comment

alyssais Feb 1, 2019

Choose a reason for hiding this comment

kou Feb 1, 2019

Choose a reason for hiding this comment

alyssais commented Feb 1, 2019 via email

kou commented Feb 1, 2019

kou left a comment

Choose a reason for hiding this comment

gsar commented May 4, 2019

kou commented May 4, 2019